feat: disambiguate input format with explicit --format option #45
Conversation
Change --format from OutputFormat (default hex) to Option<OutputFormat>. When explicitly specified, decode_input uses that format directly without fallback, preventing misdetection of base64 data as hex. When omitted, auto-detection (PEM -> hex -> base64) is preserved for backward compatibility. Output defaults to hex when --format is not specified. Add unit tests for decode_input with explicit/auto-detect modes and integration tests for explicit format roundtrip, format mismatch error messages, and default hex output verification.
Summary of ChangesHello @kiyoaki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI's robustness and user control over data encoding and decoding. By introducing an explicit Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Updates the CLI to treat --format as an explicit input encoding override (when provided), while preserving auto-detection when omitted, to prevent ambiguous hex/base64 misdetection.
Changes:
- Switched
--formatfromOutputFormat(defaulthex) toOption<OutputFormat>across commands and decoding logic. - Updated
decode_inputto honor explicit format without fallback, preserving auto-detect only when--formatis omitted. - Added unit + integration tests covering explicit format behavior, mismatch errors, and default hex output.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| kylix-cli/src/io.rs | Implements explicit-vs-auto decode behavior; adds decode helpers and unit tests. |
| kylix-cli/src/cli.rs | Makes --format optional and updates help text across commands. |
| kylix-cli/src/commands/keygen.rs | Defaults output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/encaps.rs | Defaults ciphertext/shared-secret output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/decaps.rs | Defaults shared-secret output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/sign.rs | Defaults signature output encoding to hex when --format is omitted. |
| kylix-cli/src/commands/verify.rs | Updates command signature to accept optional format. |
| kylix-cli/tests/cli_integration.rs | Adds integration tests for explicit format roundtrip, mismatch errors, and default hex output. |
| PLANS.md | Marks the M-3 plan item as implemented with the new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, introducing an explicit --format option to disambiguate input formats and prevent auto-detection errors. The change from OutputFormat to Option<OutputFormat> is applied consistently across all relevant commands. The addition of comprehensive unit and integration tests significantly improves the robustness of the input decoding logic. I have a couple of suggestions to further improve the new PEM decoding logic and Rust code style.
- Validate -----END footer in decode_pem (Copilot + Gemini) - Use generic error message listing all formats: hex|base64|pem (Copilot) - Refactor None branch to if/else if/else expression (Gemini) - Use &Path instead of to_str().unwrap() in new integration tests (Copilot) - Use input-only help text for verify's --format arg (Copilot)
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
- Extract OutputFormat::DEFAULT constant to avoid repeating Hex default across 4 command files (keygen, encaps, decaps, sign) - Add [L-6] PEM Label Validation to PLANS.md for future BEGIN/END label mismatch detection in decode_pem
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
- Handle CRLF line endings in decode_pem by normalizing \r (Copilot) - Replace OutputFormat::DEFAULT with #[derive(Default)] + #[default] and use unwrap_or_default() for idiomatic Rust (Copilot) - Fix test_default_output_is_hex: add empty check and use floor_char_boundary for safe string slicing (Copilot)
|
/gemini review |
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by disambiguating input formats through an explicit --format option. The changes correctly transition the format argument to Option<OutputFormat> across all relevant commands, allowing users to specify the encoding or rely on robust auto-detection. The decode_input function has been refactored to prioritize explicit format choices and provides helpful error messages when a format mismatch occurs. The addition of a Pem variant to OutputFormat and setting Hex as the default for output when unspecified are well-implemented. The comprehensive suite of unit and integration tests thoroughly validates the new behavior, covering explicit format decoding, fallback prevention, and auto-detection scenarios. Overall, this is a well-executed feature that enhances usability and clarity for input handling.
- Remove line continuation in error messages to avoid embedded indentation spaces in output (Copilot) - Simplify test prefix slice to trimmed.get(..40).unwrap_or(trimmed) instead of floor_char_boundary (Copilot)
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, successfully disambiguating input formats by making the --format flag optional. When provided, it strictly enforces the format, and when omitted, it maintains backward compatibility with auto-detection. The changes are consistently applied across all relevant commands. The addition of comprehensive unit and integration tests is excellent and covers the new logic well, including edge cases like hex-looking base64 data. I have a couple of minor suggestions for improving code clarity and error handling in io.rs.
…eanup - Validate PEM BEGIN/END labels match and reject malformed footers - Use Cow<str> for CRLF normalization in decode_pem - Extract format_mismatch_msg helper to deduplicate error strings - Improve auto-detect base64 fallback error to suggest --format
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement for the CLI's usability. Changing --format to an Option and using it to disambiguate input formats is a solid change that prevents misdetection, while preserving backward compatibility through auto-detection. The refactoring of decode_input and the extraction of an improved decode_pem function are well-executed. The addition of comprehensive unit and integration tests demonstrates the quality of this change. I have one minor suggestion for improving error message consistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
- Rename is_hex to is_all_hex_digits for clarity (checks characters, not valid encoding) - Add unit tests: CRLF PEM line endings, empty input auto-detection - Add integration test: explicit PEM roundtrip (encaps/decaps) - PLANS.md: move [L-6] PEM label validation to Completed, add [L-7] separate --in-format/--out-format flags
- Use strip_prefix/strip_suffix for PEM label extraction instead of
byte-index slicing to prevent panics on non-ASCII labels
- Add .context(format_mismatch_msg("pem")) to explicit PEM arm for
consistent error messages across all format branches
- Remove line-continuation backslash in format_mismatch_msg to avoid
embedded indentation whitespace in error output
- Update keygen --format help text to "Output encoding format" since
keygen has no input to auto-detect
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
There was a problem hiding this comment.
Code Review
This pull request significantly improves the CLI's input handling by allowing explicit format specification with the --format option, preventing misdetection and preserving backward compatibility. However, a critical security vulnerability has been identified in the new decode_pem function within io.rs. This function introduces intermediate non-zeroized strings that may leak secret key material in memory, contradicting the project's use of Zeroizing for sensitive data protection. Specifically, the manual CRLF normalization is redundant, and joining PEM body lines should use a protected buffer to prevent sensitive data exposure.
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces an explicit --format option to disambiguate input formats, which is a great improvement for robustness. The implementation is solid, changing the format argument to an Option and handling both explicit and auto-detection cases. The addition of comprehensive unit and integration tests is commendable. My only suggestion is to enhance the error messages in the auto-detection path to be as informative as those for explicit format failures, which will improve the user experience when decoding errors occur.
- Use with_context(|| ...) instead of context(...) to defer String allocation to the error path only - Change format list from pipe-separated to comma-separated for user-friendly CLI output - Add auto-detection context to PEM and hex error messages for consistency with base64 fallback message
External Tool Benchmark (arm64-macos)Kylix Benchmark ComparisonML-KEM-768
|
External Tool Benchmark (x86_64-linux)Kylix Benchmark ComparisonML-KEM-768
|
1 participant
Summary
--formatfromOutputFormat(defaulthex) toOption<OutputFormat>across all 5 commands--formatis explicitly specified,decode_inputuses that format directly without fallback, preventing misdetection (e.g., base64 data that looks like valid hex)--formatis omitted, auto-detection (PEM -> hex -> base64) is preserved for backward compatibility--formatis not specifieddecode_inputcovering explicit format, fallback prevention, and auto-detect modesImplements [M-3] from PLANS.md.
Test plan
cargo fmt --all -- --checkpassescargo clippy --workspace --all-features -- -D warningspassescargo clippy --workspace --no-default-features -- -D warningspassescargo test --workspace --all-featurespasses (33 tests, 0 failures)